Fix/batch verify returns failed ids - #606
Conversation
|
@AbuJulaybeeb Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
davidmaronio
left a comment
davidmaronio
left a comment
There was a problem hiding this comment.
the core of this is solid. the (verified_ids, failed_ids) tuple matches what #442 asked for, the partial-failure test with the nonexistent id 999 in test_voting.rs:370 is exactly the right coverage, and dropping the revert-on-first-error behavior fixes the wasted-gas problem described in the issue.
blockers before merge:
- this branch is stacked on #605, so it silently carries all of that pr's unrelated changes: the
CategoryMaxGoalCapfeature in src/admin.rs:536, src/storage.rs:82, and src/lib.rs:361 (which nothing enforces), plus theupdate_campaign_descriptionchange in src/campaigns/update.rs:63. please rebase onto main so this pr only contains the verify_campaigns change and its tests. - src/lib.rs:255 - the
campaigns_bulk_verifiedevent still publishes(verified, total). since callers of the event stream can't see failures either, consider includingfailed_ids.len()in the payload and updating EVENT_PAYLOADS.md (that's what #720 does for the same issue). - fyi #720 is an open duplicate of this. worth coordinating on which one lands, since the return-type change is breaking for any existing caller and only one can merge.
e70192b to
3fff301
Compare
|
this is still stacked on #605, the diff carries update.rs and contributions.rs including the duplicated token transfer that is failing ci on both branches. the ask from the last review stands: rebase onto main so this pr is only the verify_campaigns tuple change, its tests, and the event payload update. happy to re-review right after. |
9152a27 to
bdd9fe6
Compare
81f4cb6 to
8f39166
Compare
fixed please merge |
2 participants
This PR resolves #458 by updating the return signature of verify_campaigns.
Problem: Previously, verify_campaigns captured the first error it encountered but continued processing the batch. At the end, it returned the first error, which would cause the entire transaction to revert, discarding all successful verifications in that batch (wasting gas) and leaving callers with no way to identify which specific campaigns failed or succeeded.
Changes Included:
src/lib.rs: Updated verify_campaigns to return Result<(Vec, Vec), Error>. It now collects successfully verified IDs and failed IDs into separate vectors and returns them.
src/tests/test_voting.rs & src/tests/test_lifecycle.rs: Updated test assertions to expect the new (verified_ids, failed_ids) return type instead of erroring out on partial batch failures.
Acceptance Criteria Verified:
verify_campaigns now returns a tuple of (verified_ids, failed_ids).
Callers can now inspect precisely which campaigns failed verification.
Tests have been updated and validated.
Linked issue correctly.
How to Test:
bash
cargo test
closes #458